Start testing component compilation #154
Conversation
NullVoxPopuli
force-pushed
the
test-component-outputs
branch
2 times, most recently
from
176e1a7 to
fc2a517
Compare
NullVoxPopuli
force-pushed
the
add-fixture-utils
branch
from
d0df66d to
4a82bb9
Compare
NullVoxPopuli
force-pushed
the
test-component-outputs
branch
2 times, most recently
from
72237eb to
5aeec9b
Compare
NullVoxPopuli
changed the title
tests/smoke-tests/within-existing-monorepo/custom-locations.test.ts
Outdated
Show resolved
Hide resolved
…is batch of work as well as what's planned
…s (the root), so the .prettierignore *is* needed in the tests directory
NullVoxPopuli
force-pushed
the
test-component-outputs
branch
from
0c1ffd0 to
e20f37a
Compare
| let { name } = await createAddon({ | ||
| args: [`--pnpm=true`, '--addon-only'], | ||
| options: { cwd: tmpDir }, | ||
| }); | ||
|
|
||
| cwd = path.join(tmpDir, name); |
There was a problem hiding this comment.
Nit, and unrelated to this PR really, but as I'm seeing this code here... createAddon receives tmpDir, and returns name, so has everything to compute cwd, so could we do this to simplify this a bit?
| let { name } = await createAddon({ | |
| args: [`--pnpm=true`, '--addon-only'], | |
| options: { cwd: tmpDir }, | |
| }); | |
| cwd = path.join(tmpDir, name); | |
| let { name, addonDir: cwd } = await createAddon({ | |
| args: [`--pnpm=true`, '--addon-only'], | |
| options: { cwd: tmpDir }, | |
| }); |
| import { matchesFixture } from './assertions.js'; | ||
| import { copyFixture, runScript } from './utils.js'; | ||
|
|
||
| export class AddonFixtureHelper { |
There was a problem hiding this comment.
I don't want to overcomplicate things, but with #151 providing some testing infrastructure, should't this file and copyFixture ideally been part of that PR rather? 😅
Another though here, and I am totally fine ignoring this for now, like doing later or not doing at all, but given we have a more elaborate testing class here (i.e. it own some state), does it make sense to integrate createAddon into this? Like rename to e.g. TestAddonBuilder and do the addon creation as part of the constructor? Just thinking aloud here...
There was a problem hiding this comment.
should't this file and copyFixture ideally been part of that PR rather?
yes! I will move it over there -- I have a fear of making PRs too big -- especially when utils-prs really need usages to see how the utils will be used / what their purpose is.
does it make sense to integrate createAddon into this? Like rename to e.g. TestAddonBuilder and do the addon creation as part of the constructor?
I really like this idea 🎉 will do
| it('generates correct files with no template', async () => { | ||
| await addonOnlyJS.use('src/components/no-template.js'); | ||
| await addonOnlyJS.build(); | ||
| await addonOnlyJS.matches('dist/components/no-template.js'); |
There was a problem hiding this comment.
Tbh I am not sure if I would want these kind of snapshot tests against dist-output. Wouldn't these test become super brittle? Like you could update something rollup or babel dependency, which makes the out put slightly different but still perfectly valid, but still you would have tests failing. Especially annoying if you have to fix renovate PRs by hand all the time...
I do see value for fixture-based tests for all the blueprint-generated files, especially those whose content cannot be covered by smoke tests, like e.g. correct path, package manager calls etc. in the generated CONTRIBUTING.md.
But for the dist-output we don't have to care what the content really is, we only need to care if it works as expected. And we could cover this by making our smoke tests more detailed, like having different forms of components (TO, colocated, <template> and whatnot), and having a test (by means of a fixture?) in the generated addon that would cover this when running the smoke test, right?
There was a problem hiding this comment.
Yeah, these are good points -- my thinking with these tests is that they'd be faster than full smoke tests.
But maybe it's still worth it?
Because ultimately we do still want to make sure that each compiled component can be rendered.
I will adjust to more e2e-style
| console.debug(`Debug test repo at ${tmpDir}`); | ||
|
|
||
| let { name } = await createAddon({ | ||
| args: [`--pnpm=true`, '--addon-only'], |
There was a problem hiding this comment.
Why are we doing this for addon-only here? When setting up new tests, shouldn't we just start with the default scenario first?
There was a problem hiding this comment.
related to here i was focusing on compiled output, rather than e2e "workingness", if that makes sense -- I'm going to shift to the default scenario tho, and make sure that each type of component can be rendered in the test-app
NullVoxPopuli
force-pushed
the
add-fixture-utils
branch
3 times, most recently
from
387847b to
5d2aae9
Compare
|
Going to close this while I address PR feedback in a separate PR, vie existing smoke tests |
Builds on top of: #151
tests that various component scenarios are generated correctly
(basically snapshot testing with files)
This will allow us to feel more comfortable with switching up the rollup config without worrying about breaking stuff in the future.
In an effort to keep PRs small, and avoid overwhelming reviewers, I've kept the number of tests added to a small amount, and will add more in follow up PRs.
In particular, we need: